Should sf payments integration be enabled by default? It won't work with the empty sdkUrl and metadataUrl anyways.

When I run the template as is, the checkout is still operating.. it falls back to the traditional checkout. How about if we set enabled=false initially? Once the users finish the configuration, they should have already set the enabled=true.

@vmarta, so its a combination type of switch. Even if sfPayments.enabled=true in pwakit, the API returns the feature toggle value. When both are true, then its enabled. If either one of them are false, its not enabled.
Do you prefer that its always false and users have to enable it again in PWA even if FT is true?

Which ODS are you using since SF Payments needs to be configured as well. I wasn't sure how to add the value to the sdkUrl and metadataUrl and thought maybe we just document that. The handlebar file has the comments and I can add the comments here too.

So, for any internal ODS, it will look like this but someone told me that in production vanity urls can be used. So, I thought maybe I need to leave it empty and at most add comments. But we can discuss further tomorrow
sdkUrl: 'https://zyoe-011.unified.demandware.net/on/demandware.static/Sites-Site/-/-/internal/jscript/sfp/v1/sfp.js',
metadataUrl:
'https://zyoe-011.unified.demandware.net/on/demandware.static/Sites-Site/-/-/internal/metadata/v1.json'

@vmarta So, this was the mental model I had and please let me know if this is not how it should work

If I leave it to false, then every customer who wants sf payments needs to enable it in 2 different places (in feature toggle and in pwa)
If I leave it to true, then customer has the capability to use sf payments but it will only be visible if the FT is enabled by merchant
If I remove this flag completely and then depend on just the FT, then a customer has no kill switch to disable their instance if they wanted to.

Thanks for the explanation. The mental model that you've just written should be what's in the comment. It's much clearer :) Yes, please update the comment in all the config files (both in template and handlebars).

Having said that, let me ask you another question: if sf payment is enabled by default, let's say the FT is also enabled. Would the feature still work if the sdkUrl is empty?

I believe it won't work, and that's why I wish that it's enabled=false by default. When the users enter their sdkUrl value, that's when they flip the enabled to true.

Yes, I set it to false and added a bit more comments

What happens if a project does not use sf payments feature? How costly is this call to useSFPayments for every call to useCurrentBasket?

Please consider changes to useCurrentBasket to be mindful of those users not using sf payment.

Let me take a look again. Most of the stuff in useSFPayments hook are lightweight BUT useQuery for payment-metadata can be more expensive. It makes a fetch to /api/payment-metadata, even when SF Payments isn't configured. That call actually is made from ssr.js. But even though the fetch happens during SSR (so it's not a client-side network cost), it's still an unnecessary server-side fetch during the SSR process for every page render on projects that don't use SF Payments. That adds latency to the server-side render for no reason. It might not be as bad as client-side. What we can do is perhaps add some kind of short circuit in the useSFPayments hook, basically check if payments is enabled (config flag = true and api value = true) and then only process it. Will need to test it out

Related to the previous comment, calling useSFPayments hook with empty configuration results in noisy errors logged in the browser console.

useQuery for /api/payment-metadata has no enabled guard, so it fires on every page load even when sdkUrl/metadataUrl are empty
(the default). The SSR proxy then tries new URL('') on the empty metadataUrl and returns a 500 — harmless to the user but noisy in the
console.

The other two side effects are already self-gating:

• useScript(sdkUrl) — has if (!src) { return } internally, no script tag injected
• useEffect for SDK init — gated on status.loaded && typeof window.SFPayments === 'function', never runs if script didn't load

Only useQuery needs a guard. Suggested fix in use-sf-payments.js:

 export const useSFPayments = () => {
     const appOrigin = useAppOrigin()
     const config = getConfig()
     const sdkUrl = config?.app?.sfPayments?.sdkUrl
+    const metadataUrl = config?.app?.sfPayments?.metadataUrl
     const status = useScript(sdkUrl)

     ...

     const {data: serverMetadata, isLoading: serverMetadataLoading} = useQuery({
         queryKey: ['payment-metadata'],
         queryFn: async () => {
             const response = await fetch(`${appOrigin}/api/payment-metadata`)
             if (!response.ok) {
                 throw new Error('Failed to load payment metadata')
             }
             return await response.json()
         },
-        staleTime: 10 * 60 * 1000 // 10 minutes
+        staleTime: 10 * 60 * 1000, // 10 minutes
+        enabled: !!metadataUrl
     })

Using URL presence as the condition (rather than localEnabled) is more precise — enabled: true with empty URLs is still an unconfigured
state, so there's nothing to fetch regardless of the flag. A true early return isn't possible here due to Rules of Hooks.

Ya, was planning on making use of enabled property as well

This matches a lot of things. Let's consider a tighter scope for this domain. Perhaps pay.google.com.

Can we tighten this too please just to be safe? Where is the payment script coming from... *.unified.demandware.net ?

With CSP urls, it turns out that we can specify the path too.

The comment on *.demandware.net in script-src says "used to load valid payment scripts in test environment." The actual SDK URL is known
from the config comments:

https://.unified.demandware.net/on/demandware.static/Sites-Site/-/-/internal/jscript/sfp/v1/sfp.js

So this could be tightened from:

• '*.demandware.net'

• '*.demandware.net/on/demandware.static/'

Similarly for connect-src and img-src. That still uses a wildcard subdomain (unavoidable since it's instance-specific) but at least limits
execution to the static file path rather than the entire domain.

The frame-src entries (*.stripe.com, *.paypal.com, etc.) can't be tightened with paths — that's a spec limitation, not a PR issue.

One More Caveat

Path restrictions are bypassed by redirects — if a script at an allowed path issues a redirect to a disallowed path, some browsers will
still allow it. So path scoping is defense-in-depth, not a hard guarantee.

What do we want to re: this TODO?

Ya, removed that TODO

Could you explain more why bypassing ssl verification is needed? At first glance, this sounds like something we shouldn't do.

hmm I think I might be able to replace this with a fetch call instead of using http

Done

This function converts data into json string and then back to data. Do we really need to convert it to json first? If we know the data structure, then we can query the specific key and replace its value with the absolute url.

ya, you are right. I changed it

No authentication on this endpoint — it's publicly accessible. Is that all right?

right, you should be able to navigate to it from the BM instance since its just metadata

This is not needed because the request origin is the same.

Removed it

This SCAPI request can be expensive, especially when the backend configuration should not change that often.

On the other hand, SCAPI requests like this is cached by Tanstack Query library. But by default, it's cached only for 10 seconds.

There's the answer. The global default is 10 seconds:

  // _app-config/index.jsx
  defaultOptions: {
      queries: {
          retry: false,
          refetchOnWindowFocus: false,
          staleTime: 10 * 1000,   // ← 10 seconds
      }
  }

  So useConfigurations — which backs useSFPaymentsEnabled — goes stale after 10 seconds. Any component mounting after that triggers a
  background refetch to the Configurations API. On most page navigations that take longer than 10 seconds, it refetches.

  The inconsistency with the rest of the sfPayments hooks is noticeable:

  ┌──────────────────────────────────────────────┬─────────────────────────────┐
  │                    Query                     │          staleTime          │
  ├──────────────────────────────────────────────┼─────────────────────────────┤
  │ useConfigurations (via useSFPaymentsEnabled) │ 10s (global default, unset) │
  ├──────────────────────────────────────────────┼─────────────────────────────┤
  │ payment-metadata (in useSFPayments)          │ 10 minutes (explicit)       │
  ├──────────────────────────────────────────────┼─────────────────────────────┤
  │ use-sf-payments-country.js                   │ 30 minutes (explicit)       │
  └──────────────────────────────────────────────┴─────────────────────────────┘

  Configuration data like SalesforcePaymentsAllowed is a merchant-level setting that essentially never changes at runtime. It should carry a
  much longer staleTime, consistent with how the rest of the sfPayments hooks treat their data. useShopperConfiguration could pass a
  queryOptions argument through to useConfigurations:

  export const useShopperConfiguration = (configurationId) => {
      const {data: configurations} = useConfigurations(
          {},
          {staleTime: 30 * 60 * 1000}  // treat config as long-lived
      )
      ...
  }

  Or set it at the useSFPaymentsEnabled call site via the second queryOptions argument that useShopperConfiguration currently doesn't expose.
   Either way, leaving it at the 10-second global default means the Configurations endpoint gets hit on virtually every page load for all
  users.

See this comment for a possible way to address the above caching: #3709 (comment)

I added a cache time for now for that userShopperConfig call

Oh I see useShopperConfiguration is a custom hook that is defined in the template (not in commerce-sdk-react). So we can tweak it to allow additional options like staleTime.

  -export const useShopperConfiguration = (configurationId) => {
  -    const {data: configurations} = useConfigurations()
  +export const useShopperConfiguration = (configurationId, queryOptions = {}) => {
  +    const {data: configurations} = useConfigurations(undefined, queryOptions)
       const config = configurations?.configurations?.find(
           (configuration) => configuration.id === configurationId
       )
       return config?.value
   }

  Then the caller decides the caching behaviour — useSFPaymentsEnabled would become:

  export const useSFPaymentsEnabled = () => {
      const config = getConfig()
      const localEnabled = config?.app?.sfPayments?.enabled ?? true
      const apiEnabled =
          useShopperConfiguration('SalesforcePaymentsAllowed', {
              staleTime: 30 * 60 * 1000
          }) === true
      return localEnabled && apiEnabled
  }

  This keeps the hardcoded value out of the hook implementation, lets other callers (useAutomaticCapture, useFutureUsageOffSession) set their
   own staleTime if needed, and aligns exactly with the pattern already used everywhere else in the package.

For what reasons do we need these domains? We already have pay.google.com and payments.google.com above.

Removing google.com / www.google.com can very likely break the Google Pay flow, even though the button itself still renders. There tend to be 3DS and other redirections that google pay does during the flow and we need to make sure that none of those break. Thats why we left them in connect source but not in script source.

I saw these in the browser
Connecting to 'https://google.com/pay' violates the following Content Security Policy directive: "connect-src ..."
This means the Google Pay script successfully loads, but when it tries to communicate with Google’s payment service, the request to google.com is blocked by CSP.

We can do some thorough testing after the merge and once its in the right environments and discuss it with others in the team before updating it further

@amittapalli we can update it to be google.com/pay/ then. As this previous comment said, we can control the path too.. not just the domains.

After the merge, please update it to be more narrow scoped.

NIT, for all comment headers in the new ShopperBasketsV2 directory:

In both mutation.ts and query.ts there we should update all typedoc comments

Let's differentiate V1 and V2 when we run our unit tests, we should apply this change in all the new test files in the ShopperBasketsV2 directory if applicable, based on my search we should update:

• index.test.ts
• mutation.test.ts
• query.test.ts

Currently with this version of queryKeyHelpers.ts, If a developer uses both useBasket (V1) and useBasketV2 (V2) during a migration period, they will share the exact same TanStack Query cache entry. A V2 mutation will update/invalidate V1 query caches and vice versa. I'm not sure how likely this scenario will happen where a customer mixes and matches V1 and V2, but to be on the safe side and to decouple V1 and V2 completely, we should update this implementation so that V2 endpoints target different cache keys than V1 endpoints:

For all keys

We can take a look at this post merge. How likely is it that someone uses V1 and V2 simultaneously? If it's extremely unlikely and this is a time-sensitive merge, it might be something to address in a follow-up